Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add hover states for buttons #620

Merged
merged 15 commits into from
Nov 13, 2024
Merged

Add hover states for buttons #620

merged 15 commits into from
Nov 13, 2024

Conversation

guergana
Copy link
Collaborator

@guergana guergana commented Oct 30, 2024 •
edited
Loading

Hover states done:

  • Upload your data sidebar ✔️
  • Upload your data empty screen ✔️
  • Create folder Dialog ✔️
  • Publish Dialog ✔️
  • File Upload Dialog - Add External Data Tab ✔️
  • Launch Screen (aka Welcome Banner) ✔️
  • How the ODE saves your data ✔️
  • Publish button upper right bar (missing design) ✔️
  • Unsaved changes ✔️
  • Rename file/folder dialog ✔️
  • Delete dialog ✔️

@guergana guergana changed the title Add hover states for buttons [WiP] Add hover states for buttons Oct 30, 2024
@guergana guergana marked this pull request as draft October 30, 2024 20:34
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Oct 31, 2024 •
edited
Loading

Deploying opendataeditor with  Cloudflare Pages  Cloudflare Pages

Latest commit: f044384
Status: ✅  Deploy successful!
Preview URL: https://9f4aae10.opendataeditor.pages.dev
Branch Preview URL: https://596-hover-states.opendataeditor.pages.dev

View logs

guergana and others added 7 commits October 31, 2024 16:46
@guergana
Add blue background to two buttons dialogs
58e84c3
@pdelboca
278 - MacOS Notarize (#621)
1c73fd8 
* 278 - Add MacOS notarize files and secrets

* Fix typo

* Allow notarize in PRs

* Add notarize secrets to the action

* Upgrade electron-builder

* Add some extra resources to the file

* Clean debugging option
@guergana
Add 'Rename' option to file Context menu (#613)
da28009
@guergana
Add error counter to menu bar errors Report Button (#611)
c7fd20e
@guergana
Add hover states for buttons
6ed51ec
@guergana
Adjust styles for Publish, Save changes, Upload data buttons
28980d5 
Also welcomeBanner and FileUpload
@guergana
Add blue background to two buttons dialogs
65f232c
@guergana guergana marked this pull request as ready for review November 6, 2024 00:55
@guergana guergana changed the title [WiP] Add hover states for buttons Add hover states for buttons Nov 6, 2024
@roll
Copy link
Collaborator

roll commented Nov 6, 2024

Hi, looks good!

A few things I spotted:

The hover border doesn't seem to be from the palette:

hover

Publish button looks to be in the wrong place:

publish

Not related to this PR I think but the "Unsaved Changes" dialog is really unclear:

changes

Cancel the action or cancel the changes? Usually, in this context cancel means don't do anything and get me back (not discard)

@guergana
Copy link
Collaborator Author

guergana commented Nov 8, 2024

Hi, looks good!

A few things I spotted:

The hover border doesn't seem to be from the palette:

hover

Publish button looks to be in the wrong place:

publish

Not related to this PR I think but the "Unsaved Changes" dialog is really unclear:

changes

Cancel the action or cancel the changes? Usually, in this context cancel means don't do anything and get me back (not discard)

Hi @roll I have addressed your comments except the one from the cancel. What is your suggestion? To have the text say "discard" instead of cancel? We wanted to have a standard cancel action for all buttons, this is why we changed the discard text to have all say "cancel" but you are right, in this case is confusing. This should be discussed with @romicolman and @Faithkenny .

romicolman
romicolman approved these changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@romicolman romicolman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I checked everything on Mac. All dialogs are now OK except from the Publish one. I tried to add an URL and the button never gets activated. I think this is a separate issue so if the button is aligned with the rest of the design, please, merge it and I'll create a separate ticket.

@romicolman
Copy link
Collaborator

Hi! I have just checked all comments. Please, do not change the Cancel button for Discard. However, let's make the text clearer:

Replace

There are unsaved changes.

For

There are unsaved changes. Please, click save or cancel.

@roll
Copy link
Collaborator

roll commented Nov 11, 2024

Please, click save or cancel

Cancel action or changes?

@romicolman
Copy link
Collaborator

Hi! Actions can be broad for users. Please @guergana let's procede with the text I suggested until we finish the user testing session. I'll inform you both if users mention find the change confusing.

@guergana guergana merged commit aa38f04 into main Nov 13, 2024
9 checks passed
@guergana guergana deleted the 596-hover-states branch November 13, 2024 22:00
@guergana guergana mentioned this pull request Nov 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romicolman romicolman romicolman approved these changes

@pdelboca pdelboca Awaiting requested review from pdelboca

@roll roll Awaiting requested review from roll

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hover state consistency in the Modals/Dialogs across the ODE
4 participants
@guergana @roll @romicolman @pdelboca